Repository navigation
fix(eval): resolve carla_cosim benchmarks and lint for unresolved names - #14
Conversation
|
Gate 1 note — the two red checks are not this PR's.
It is fixed by #13, which is now 12/12 green. Once #13 is stacked and this branch is rebased, Gate 1 can be satisfied here. |
d91b4b2 to
cef3a07
Compare
67ba999 to
ca69da8
Compare
cef3a07 to
17e264a
Compare
ca69da8 to
d6b24e6
Compare
dafe1bd to
74a648b
Compare
Selecting the shipped carla_cosim benchmark crashed with UnboundLocalError: the cosim branch of load_benchmark_config appended to resolved_benchmarks, which is not assigned until after the loop it sits in. The parsed cosim entries now accumulate in their own list and are appended to the normalized replay benchmarks, so a selection may mix both kinds. The bug is the kind a linter finds for free, so the ruff hook that has been commented out since it was added is enabled, scoped to exactly that class of defect: names that do not resolve and imports that are never used. The repo has 104 other violations under ruff's full default set, most of them E402 import placement, and fixing those is neither this change nor obviously desirable; a deliberately narrow select keeps the hook green today and lets the set widen later alongside the fixes a wider set would demand. Enabling it removed eight genuinely unused imports and required noqa markers on the four import-availability probes that exist precisely to be unused. tests/unit_tests/test_terminate_on_goal.py called pytest.main without importing pytest, so its __main__ entry point had never worked.
d6b24e6 to
bb5d3b0
Compare
jdsika
left a comment
There was a problem hiding this comment.
Adversarial review (Track A) — posted on behalf of a fresh-context reviewer with no authoring history on this change, per .github/copilot-instructions.md §4.7 and §5.
Reviewed head bb5d3b03e; its patch is byte-identical to the originally reviewed d6b24e69d (the rebase changed only the base).
Verdict: approve as-is
No blocking, should-fix or optional code findings.
What was probed
Including the probes that found nothing, as §4.7 requires.
- Every source hunk — before/after AST comparison. Outside the benchmark-resolution fix and its new test, executable statements are unchanged apart from imports.
- The original crash — executing the unfixed base reproduces
UnboundLocalErrorwhen selecting the shippedcarla_cosimbenchmark. The fix has a test that fails without it. - Regression suite — the co-simulation contract module executed from immutable source blobs: 11 tests passed.
- Mixed selections — co-sim-only, native-only, both selection orders, interleaved, and duplicated. All resolve.
- Ordering — mixed selections return native benchmarks first, then co-sim, rather than the original cross-type selection order. Traced through the evaluator; no correctness or numerical regression established.
- Replay compatibility — native-only results compared against the base, including map-directory and scenario-count overrides, duplicates and whitespace-separated selections. Identical.
- Determinism and mutation — repeated mixed resolution returns identical values, preserves both the Python and NumPy RNG states, and does not mutate the input mapping.
- Fail-fast input handling — empty/invalid selections, unknown names, duplicate benchmark definitions, missing environment mappings, malformed and empty route lists, nonpositive ports and unsupported environment keys all raise explicit errors.
- Hot paths — no change to C stepping, observations, rewards, policy computation or training batching. The new list accumulation happens during configuration loading.
- Removed imports — contexts, retained dependency imports and consumers inspected. No lost initialisation side effect and no required re-export.
- Suppressions — all five added annotations are scoped to
F401availability/initialisation imports or thePolicyexport. No per-file or sim-wide suppression was added. - Rule selection — the pinned ruff flags the original defect as
F821and duplicate classes asF811. The selection is narrow enough not to impose unrelated restructuring. - Full lint and format — all 164 tracked Python blobs. On the rebased head: all checks pass, 164 files already formatted.
- Hook wiring — lint precedes formatting; both hooks pin
v0.12.12, share file-type coverage and discover the root configuration; both retain Python 3.10 / 120 columns. No version skew. - Automatic fixes — unused imports are removable, annotated probes survive, and the duplicate-class defect is not auto-deleted with unsafe fixes disabled.
pre-commitrejects a commit when hooks modify files rather than silently staging rewrites. - Merge-result CI — the earlier red check contained only the
F811inherited from the base, since fixed there. The rebased merge tree equals the reviewed head's tree and its fullpre-commitworkflow passes, clang-format included.
Stated verification limits: the source-rewriting pre-commit run --all-files was not executed locally (check-only commands plus the real CI run were used instead), and real CARLA / SLURM / nuPlan execution was not exercised.
Track
forkctl track --base 74a648b67 --head bb5d3b03e reports A, forced by six changed paths under pufferlib/. The reviewer agrees.
What
Fix an
UnboundLocalErrorthat makes every benchmark selection containingcarla_cosimcrash, andenable the
rufflint hook — scoped to exactly the class of defect that bug belongs to — so thenext one is caught by a tool instead of a reviewer.
Why
load_benchmark_configappends parsed co-sim benchmarks toresolved_benchmarks, which is notassigned until after the loop that does the appending:
pufferlib/config/evaluation/benchmark.yamlships acarla_cosimbenchmark, so this is reachablefrom the repo's own config, not a hypothetical:
Parsed co-sim entries now accumulate in their own list and are appended to the normalized replay
benchmarks, so a selection may mix both kinds — which is what the
continuewas always reaching for.The second half is the reason this is one PR and not two. The bug is a name that does not resolve:
a linter finds it for free, and the
ruffhook that would have found it has been sitting commentedout in
.pre-commit-config.yamlsince it was added. Enabling it is what turns the fix into a classof bugs that cannot come back, and the fix is what lets the hook be enabled at all.
Why the lint is narrow
Under ruff's full default rule set the repo has 104 violations. 56 of them are
E402(importsnot at top of file), which is mostly deliberate
sys.pathmanipulation in scripts and would needreal restructuring; the rest include 26
F841and 8E722, where "fixing" them means changingexception and binding semantics in code this PR has no business touching.
So
ruff.tomlselects a deliberately narrow set —E9,F401,F811,F821,F822: names thatdo not resolve, and imports that are never used. That is green today, it is the tier where a hit is
almost always a genuine defect rather than a style opinion, and widening it later is a decision that
can be taken alongside the fixes a wider set would demand.
How tested
pre-commit run --all-filesmake rebuild(required if.c/.hchanged — zero new warnings)make test-unit/make test-evalmake test-c(C sim logic)make test-notebooks(notebooks touched)make test-docker-smoke(dynamics, rewards, observations, or training loop touched)make -C tests/drive perf(hot paths touched)make testbefore marking ready (unit + eval + c + notebooks — does not includedocker-smoke or perf; those are the two boxes above)
The 4 failures are pre-existing and unrelated: 3 need
pyarrow, which is not installed in thischeckout's venv, and 1 is the
fused_slot_encoder/triton breakage fixed in #13. Confirmed byrunning the same 4 on a clean tree with this commit stashed — identical failures, so this change
introduces none of them.
make test-notebookscould not run here (nbclientmissing), but this commit removes two unusedimports from
notebooks/notebook_utils.pyandpufferlib/ocean/cosim/carla_cosim.py; theNotebook Execution check covers it.
New behaviour is covered by a new test / the fix has a test that fails without it:
tests/unit_tests/test_cosim_config_contract.py::TestCosimBenchmarkResolution::test_shipped_carla_cosim_benchmark_resolves— it loads the shipped
benchmark.yamland selectscarla_cosim. Verified it fails withoutthe fix and passes with it:
Stack hygiene
forkctl check-commitpasses (--choreon a fork-only branch)integration/3.0Merge gate
explicitly accepted in the thread
forkctl mirror <slug> --sha HEADpasses, i.e. thecommit cherry-picks onto pure
upstream/3.0and its diff is upstream-cleanGate 3 result:
Upstream disposition
Emerge-Lab/PufferDriverequested now — needs a human to verify the finaltitle, body and diff first, and the body must carry the authorship disclosure from
.github/fork/PROCESS.md→ Agent authorshipcleanly to pure
upstream/3.0, but opening an upstream PR needs explicit human approval ofthe title, body and diff (§5). The lint half is more of a judgement call for them than the
fix half, and may be worth splitting if they want only one of the two.
chore(fork):change, Gate 3 exemptRisk / notes
Enabling the hook removed 8 genuinely unused imports across
data_utils/download_womd_data.py,notebooks/notebook_utils.py,pufferlib/ocean/cosim/carla_cosim.py,pufferlib/ocean/evaluation_utils/wosac/estimators.py,scripts/eval/analyze_nuplan_cosim.pyand
tests/unit_tests/test_map_metrics.py. Each was reviewed by hand: none is a re-export andnone has an import side effect, so removing them is inert.
Four import-availability probes gained
# noqa: F401—gymnasium/pygameinpufferlib/__init__.py,torchand thePolicyre-export inpufferlib/ocean/__init__.py, andpufferlib._Cinpufferlib/pufferl.py. These exist precisely to be imported and not used, so themarker is the honest answer rather than a restructure.
tests/unit_tests/test_terminate_on_goal.pycalledpytest.mainwithout importingpytest,so its
__main__entry point had never worked. Now imported, matching every other test module.The hook id is
ruff-check, not theruffalias the commented-out block used: the alias stillworks at the pinned rev but prints a deprecation warning on every run.
--fixis kept from the original commented-out config, so the hook repairs what it safely canrather than only complaining. Unsafe fixes stay off.
Ordering note: this PR and #13 are independent and both cut from
integration/3.0; whichever landsfirst, the other rebases cleanly (disjoint files).